Add aria-describedby attribute where missing for fields with error#2579
Add aria-describedby attribute where missing for fields with error#2579AbdiTolesa wants to merge 55 commits into
Conversation
WalkthroughThe changes enhance ARIA accessibility for form fields by making the Changes
Sequence Diagram(s)sequenceDiagram
participant Ctrl as FrmFieldsController<br/>(input_html)
participant FT as FrmFieldType
participant Hook as Hook Caller<br/>(frm_field_input_html)
rect rgb(200, 220, 255)
Note over Ctrl,Hook: ARIA Description Flow
Ctrl->>Ctrl: Check if field['args] set
alt args present
Ctrl->>FT: Load field type object
Ctrl->>FT: add_aria_description(args, &$html)<br/>(public method)
FT->>FT: Inject ARIA attributes into $html
FT-->>Ctrl: Return modified $html
else no args
Ctrl->>Ctrl: Use base $html
end
end
Ctrl->>Ctrl: Assign $html (unset $add_html)
Ctrl->>Hook: echo $html or return $html
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 Biome (2.1.2)css/frm_testing_mode.css[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally. text-align is already defined here. Remove or rename the duplicate property to ensure consistent styling. (lint/suspicious/noDuplicateProperties) [error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally. min-width is already defined here. Remove or rename the duplicate property to ensure consistent styling. (lint/suspicious/noDuplicateProperties) [error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally. border-radius is already defined here. Remove or rename the duplicate property to ensure consistent styling. (lint/suspicious/noDuplicateProperties) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/views/frm-fields/front-end/dropdown-field.php(1 hunks)classes/views/frm-fields/front-end/radio-field.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHPStan
classes/views/frm-fields/front-end/radio-field.php
[failure] 57-57:
Implicit array creation is not allowed - variable $field might not exist.
🔇 Additional comments (1)
classes/views/frm-fields/front-end/radio-field.php (1)
57-58: <!-- [scratchpad_start]
[task review_file_1/1 verify_args_availability 3/3 COMPLETE]
Final verification complete - $args is in scope[observations]
- Found FrmFieldType::include_on_front_form() at lines 1071-1098
- Template file included directly via: include $include_file; (line 1093)
- Variables available in template scope:
- $args (passed as parameter at line 1071, not removed)
- $field (assigned at line 1085 from $this->field)
- $html_id (assigned at line 1086)
- $field_name (assigned at line 1087)
- $read_only (assigned at line 1088)
- NO extract() call - variables made available via direct assignment
- Before include, only $args['form'] is unset (line 1090)
[analysis]
- $args parameter is passed through to include_on_front_form()
- All variables needed by template are in scope before include
- Radio-field.php line 57: $field['args'] = $args; is VALID
- $args is definitely in scope when template executes
- No undefined variable error can occur
[conclusion]
- Review comment concern is INVALID
- The code is CORRECT
- $args is proven to be in scope
- No changes needed
- All similar files (checkbox-field.php, dropdown-field.php) follow same pattern
[done]
Verification complete: NO ISSUES FOUND
No issues found. The
$argsvariable is properly in scope.The template file is included at line 1093 of
FrmFieldType::include_on_front_form()with$argspassed as a parameter and available in the local scope. Additional variables like$field,$html_id, and$field_nameare assigned before the include statement, making them available to the template. The assignment$field['args'] = $args;at lines 57-58 in radio-field.php is valid and safe.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/controllers/FrmFieldsController.php(1 hunks)classes/models/fields/FrmFieldType.php(1 hunks)classes/views/frm-fields/front-end/checkbox-field.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/views/frm-fields/front-end/checkbox-field.php
- classes/models/fields/FrmFieldType.php
🧰 Additional context used
🧬 Code graph analysis (1)
classes/controllers/FrmFieldsController.php (2)
classes/factories/FrmFieldFactory.php (2)
FrmFieldFactory(9-141)get_field_object(58-64)classes/models/fields/FrmFieldType.php (1)
add_aria_description(1362-1410)
🔇 Additional comments (1)
classes/controllers/FrmFieldsController.php (1)
527-527: Refactoring approach looks good.The introduction of the
$htmlvariable to store the constructed HTML before output is a clean approach that allows the ARIA description to be conditionally added before echoing or returning. This separation of HTML construction and output handling makes the code more maintainable.Also applies to: 534-538
|
Hi @AbdiTolesa, there are some error warnings showing in this branch. Please see the attached screenshot: |
Hi @lauramekaj1 Just fixed those. |
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@AbdiTolesa I tested it and it's working as expected. Thank you!
…e role='alert' from error description
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
classes/views/frm-fields/front-end/checkbox-field.php (1)
76-79: Passing$argsvia$field['args']into the hook is consistent and safeConditionally attaching
$argsonto$fieldbeforefrm_field_input_htmlgives hook callbacks the extra context they need (e.g., for ARIA handling) without changing the hook signature or breaking existing consumers. Theisset( $args )guard avoids notices when$argsis not provided, and reusing the mutated$fieldlater in this template remains compatible with current usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/controllers/FrmFieldsController.php(1 hunks)classes/models/FrmFieldFormHtml.php(1 hunks)classes/views/frm-fields/front-end/checkbox-field.php(2 hunks)classes/views/frm-fields/front-end/dropdown-field.php(1 hunks)classes/views/frm-fields/front-end/radio-field.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- classes/views/frm-fields/front-end/radio-field.php
- classes/views/frm-fields/front-end/dropdown-field.php
- classes/controllers/FrmFieldsController.php
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmFieldFormHtml.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4666)should_include_alert_role_on_field_errors(3684-3686)
🔇 Additional comments (1)
classes/views/frm-fields/front-end/checkbox-field.php (1)
11-16: Docblock variable annotations are clear and accurateThe added
@varannotations correctly document the template variables and their types, improving readability and IDE support without affecting runtime behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/controllers/FrmFieldsController.php(1 hunks)classes/models/FrmFieldFormHtml.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/controllers/FrmFieldsController.php
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmFieldFormHtml.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4666)should_include_alert_role_on_field_errors(3684-3686)
🪛 GitHub Check: PHPStan
classes/models/FrmFieldFormHtml.php
[failure] 236-236:
Variable $error in empty() always exists and is always falsy.
🔇 Additional comments (1)
classes/models/FrmFieldFormHtml.php (1)
239-249: LGTM! Role injection/removal logic is correct.The logic correctly handles role="alert" injection and removal:
- Line 240:
$has_alert_rolecorrectly indicates presence (fixed from previous inverted logic)- Line 241: Injects role when missing AND setting allows
- Line 247: Removes role when present AND setting disallows
The early return at line 235 ensures this block only executes when an error exists, addressing truongwp's concern from the previous review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/models/FrmFieldFormHtml.php (1)
235-237: Consider separating the function call from the return statement.The code returns the result of
FrmShortcodeHelper::remove_inline_conditions(), but this function modifies$this->htmlby reference and doesn't return a meaningful value (as seen on line 251 where it's called without capturing the return). While functionally correct, explicitly separating the call from the return improves clarity.Apply this diff for clearer intent:
if ( empty( $error ) ) { - return FrmShortcodeHelper::remove_inline_conditions( false, 'error', $error, $this->html ); + FrmShortcodeHelper::remove_inline_conditions( false, 'error', $error, $this->html ); + return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmFieldFormHtml.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmFieldFormHtml.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4666)should_include_alert_role_on_field_errors(3684-3686)
🔇 Additional comments (1)
classes/models/FrmFieldFormHtml.php (1)
239-251: LGTM! Alert role logic is now correct.The inverted logic bug has been successfully resolved. The variable
$has_alert_rolenow correctly reflects whetherrole="alert"is present in the HTML (false !== strpos(...)evaluates to true when found). The conditional branches properly inject the role when missing and enabled, or remove it when present but disabled.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/formidable.js (1)
1148-1186: Bug: aria-describedby is computed but never applied to inputs inaddFieldErrorInside
addFieldError, theinputs.forEachloop builds adescribedBystring for each input but never callssetAttribute, soaria-describedbyis not actually updated for any input. This means your JS/AJAX validation path will still fail to expose errors to assistive tech, undermining the main goal of this PR.You can fix this by making
describedBylocal to the loop and writing it back to the element:- function addFieldError( $fieldCont, key, jsErrors ) { - let id, describedBy, roleString; + function addFieldError( $fieldCont, key, jsErrors ) { + let id, roleString; @@ - container.classList.add( 'frm_blank_field' ); - const inputs = container.querySelectorAll( 'input, select, textarea' ); - id = getErrorElementId( key, inputs[ 0 ] ); + container.classList.add( 'frm_blank_field' ); + const inputs = container.querySelectorAll( 'input, select, textarea' ); + id = getErrorElementId( key, inputs[ 0 ] ); @@ - container.insertAdjacentHTML( 'beforeend', errorHtml ); - inputs.forEach( input => { - describedBy = input ? input.getAttribute( 'aria-describedby' ) : null; - if ( ! describedBy ) { - describedBy = id; - } else if ( describedBy.indexOf( id ) === -1 && describedBy.indexOf( 'frm_error_field_' ) === -1 ) { - const errorFirst = input.dataset.errorFirst; - if ( errorFirst === '0' ) { - describedBy = describedBy + ' ' + id; - } else { - describedBy = id + ' ' + describedBy; - } - } - } ); + container.insertAdjacentHTML( 'beforeend', errorHtml ); + inputs.forEach( input => { + if ( ! input ) { + return; + } + let describedBy = input.getAttribute( 'aria-describedby' ); + if ( ! describedBy ) { + describedBy = id; + } else if ( describedBy.indexOf( id ) === -1 && describedBy.indexOf( 'frm_error_field_' ) === -1 ) { + const errorFirst = input.dataset.errorFirst; + if ( errorFirst === '0' ) { + describedBy = describedBy + ' ' + id; + } else { + describedBy = id + ' ' + describedBy; + } + } + input.setAttribute( 'aria-describedby', describedBy ); + } );This preserves your existing logic (including
data-error-firstordering) while actually wiring up the attribute on every input in the group.
♻️ Duplicate comments (1)
js/formidable.js (1)
1260-1279: HardenupdateInputElementsAriaDescribedByagainst missing IDsIf an error element doesn’t have an
id,el.idis an empty string and the selector[aria-describedby*=""]will match every element with anaria-describedbyattribute. The current filter means you won’t remove anything, but you’ll still iterate over all such elements unnecessarily, and this repeats the earlier “missing null check” concern that was already raised on this PR.A small guard keeps this efficient and future‑proof:
- function updateInputElementsAriaDescribedBy( el ) { - document.querySelectorAll( `[aria-describedby*="${ el.id }"]` ).forEach( input => { + function updateInputElementsAriaDescribedBy( el ) { + if ( ! el || ! el.id ) { + return; + } + document.querySelectorAll( `[aria-describedby*="${ el.id }"]` ).forEach( input => { let ariaDescribedBy = input.getAttribute( 'aria-describedby' ).split( ' ' ); ariaDescribedBy = ariaDescribedBy.filter( value => value.trim() && value !== el.id );(You can keep the rest of the function as is; the token filtering already avoids substring false positives.)
🧹 Nitpick comments (1)
js/formidable.js (1)
1281-1289: Consider also clearingaria-invalidwhen removing all errors
removeAllErrorsnow correctly cleans uparia-describedbyviaupdateInputElementsAriaDescribedBy, but it doesn’t resetaria-invalid. After an AJAX submit where some fields were previously invalid, those fields can keeparia-invalid="true"even when no.frm_errorremains, which is misleading for assistive tech.You could optionally normalize this here:
function removeAllErrors() { document.querySelectorAll( '.form-field' ).forEach( field => { field.classList.remove( 'frm_blank_field', 'has-error' ); } ); + document.querySelectorAll( '.form-field [aria-invalid="true"]' ).forEach( el => { + el.setAttribute( 'aria-invalid', 'false' ); + } ); document.querySelectorAll( '.form-field .frm_error' ).forEach( el => { updateInputElementsAriaDescribedBy( el ); el.remove(); } );Also, the new JSDoc above
updateInputElementsAriaDescribedBystill uses@since x.x; updating that to the actual release version when known will keep the docs consistent with the rest of the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
|
@AbdiTolesa I'm still not seeing |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/formidable.js (2)
1170-1183: Questionable logic in error ID existence check.Line 1174 checks if
describedBy.indexOf( 'frm_error_field_' ) === -1, which prevents adding the current error ID if any error field ID pattern is already present. This could cause issues when errors are updated but not fully cleared first, as the new error ID won't be added if an old error ID remains.Consider checking only for the specific error ID (
id) rather than the generic pattern, or explicitly replace any existing error IDs with the new one.Example scenario: If field has
aria-describedby="frm_error_field_123 field_description"and a new error with IDfrm_error_field_456is added, the new ID won't be added due to this check.
1222-1259: Inconsistent multi-input handling in removeFieldError.While
addFieldErrornow correctly handles multiple inputs (lines 1170-1183 and 1186-1195),removeFieldErrorstill only processes the first input found byquerySelectoron line 1230. This creates an inconsistency for checkbox/radio groups when using JS validation:
- When an error is added, all inputs in the group get
aria-describedby- When the error is cleared, only the first input has
aria-describedbycleaned up- Other inputs in the group retain references to removed error elements
This directly relates to the reported gap: "inputs did not show aria-describedby when JS validation is enabled."
Consider updating
removeFieldErrorto handle multiple inputs similar toaddFieldError:const errorMessage = container.querySelector( '.frm_error' ); const errorId = errorMessage ? errorMessage.id : ''; - const input = container.querySelector( 'input, select, textarea' ); - let describedBy = input ? input.getAttribute( 'aria-describedby' ) : null; + const inputs = container.querySelectorAll( 'input, select, textarea' ); container.classList.remove( 'frm_blank_field', 'has-error' ); - if ( input ) { - if ( 'true' === input.getAttribute( 'aria-invalid' ) ) { - input.setAttribute( 'aria-invalid', 'false' ); - } else if ( [ 'radio', 'checkbox' ].includes( input.type ) ) { - const group = input.closest( '[role="radiogroup"], [role="group"]' ); - if ( group ) { - group.setAttribute( 'aria-invalid', 'false' ); - } - } - } + inputs.forEach( input => { + if ( 'true' === input.getAttribute( 'aria-invalid' ) ) { + input.setAttribute( 'aria-invalid', 'false' ); + } else if ( [ 'radio', 'checkbox' ].includes( input.type ) ) { + const group = input.closest( '[role="radiogroup"], [role="group"]' ); + if ( group ) { + group.setAttribute( 'aria-invalid', 'false' ); + } + } + } ); if ( errorMessage ) { errorMessage.remove(); } - if ( input ) { - input.removeAttribute( 'aria-describedby' ); - if ( describedBy ) { - describedBy = describedBy.replace( errorId, '' ).trim(); - if ( describedBy ) { - input.setAttribute( 'aria-describedby', describedBy ); - } - } - } + inputs.forEach( input => { + let describedBy = input.getAttribute( 'aria-describedby' ); + if ( describedBy ) { + describedBy = describedBy.replace( errorId, '' ).trim(); + if ( describedBy ) { + input.setAttribute( 'aria-describedby', describedBy ); + } else { + input.removeAttribute( 'aria-describedby' ); + } + } + } ); }
🧹 Nitpick comments (1)
js/formidable.js (1)
1261-1280: Well-implemented ARIA cleanup with minor safety suggestion.The helper correctly removes the specific error ID from
aria-describedbywhile preserving other IDs (like field descriptions), and handles multiple inputs properly.For maximum safety, consider a null check on line 1271:
document.querySelectorAll( `[aria-describedby*="${ el.id }"]` ).forEach( input => { - let ariaDescribedBy = input.getAttribute( 'aria-describedby' ).split( ' ' ); + let ariaDescribedBy = ( input.getAttribute( 'aria-describedby' ) || '' ).split( ' ' ); ariaDescribedBy = ariaDescribedBy.filter( value => value.trim() && value !== el.id );This prevents potential null reference errors if the attribute is removed between query and access, though the selector makes this scenario unlikely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
js/formidable_admin.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_blocks.js.mapis excluded by!**/*.map,!**/*.map
📒 Files selected for processing (1)
js/formidable.js(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (4)
js/formidable.js (4)
1156-1157: LGTM: Multi-input collection for better accessibility.The change correctly switches from handling a single input to collecting all inputs within the field container, which properly supports checkbox and radio groups for ARIA attribute management.
1186-1195: LGTM: Proper ARIA invalid state handling.The implementation correctly applies
aria-invalidto radio/checkbox groups (on the parent container with role) versus individual inputs, following ARIA best practices.
1207-1213: LGTM: ESLint spacing issues resolved.The bracket spacing now correctly matches the file's style guide with spaces inside brackets (
[ 0 ]), and the ID generation logic is consistent across both branches.
1286-1289: LGTM: Proper error cleanup with ARIA synchronization.The updated
removeAllErrorscorrectly calls the new helper to clean uparia-describedbyattributes before removing error elements, ensuring screen readers get updated state. The more specific selector (.frm_form_field) is also appropriate.
AbdiTolesa
requested review from
Crabcyborg
and removed request for
Crabcyborg
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/models/fields/FrmFieldType.php (1)
1560-1620: Public visibility change is reasonable, but please verify child overrides and finalize the@sinceversion.Making
add_aria_descriptionpublic is a sensible way to reuse the ARIA logic fromFrmFieldsController::input_html, and the method body remains unchanged, so behavior is consistent.Two follow‑ups:
Check for subclasses with narrower visibility.
If any class extendingFrmFieldTypeoverridesadd_aria_descriptionasprotected, this will now conflict with the parent’spublicvisibility and cause a fatal error when the class is loaded. Please scan the codebase for overrides and update them topublicif they exist.Replace the placeholder version in the docblock.
@since x.x Function privacy changed from \protected` to `public`.` should use the actual release version before merging.You can use this script to look for overrides:
#!/bin/bash # Find all implementations of add_aria_description in the plugin. rg -nP 'function\s+add_aria_description\s*\('
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/fields/FrmFieldType.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/fields/FrmFieldType.php (1)
classes/controllers/FrmFieldsController.php (1)
input_html(531-563)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
classes/models/fields/FrmFieldType.php (1)
1543-1543: Replace version placeholder in @SInCE tag.The
@since x.xplaceholder should be replaced with the actual version number for this release.js/formidable.js (2)
1156-1157: Add safety check for empty inputs array.If a field container has no inputs (edge case), accessing
inputs[0]on line 1157 will returnundefined, potentially causing issues ingetErrorElementId().🔎 Proposed fix
container.classList.add( 'frm_blank_field' ); const inputs = container.querySelectorAll( 'input, select, textarea' ); + if ( ! inputs.length ) { + return; + } id = getErrorElementId( key, inputs[ 0 ] );
1269-1280: Add validation for error element ID.If
el.idis empty or undefined, the selector on line 1270 will be malformed (e.g.,[aria-describedby*=""]), potentially matching unintended elements.🔎 Proposed fix
function updateInputElementsAriaDescribedBy( el ) { + if ( ! el.id ) { + return; + } document.querySelectorAll( `[aria-describedby*="${ el.id }"]` ).forEach( input => { let ariaDescribedBy = input.getAttribute( 'aria-describedby' ).split( ' ' ); ariaDescribedBy = ariaDescribedBy.filter( value => value.trim() && value !== el.id );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/controllers/FrmFieldsController.phpclasses/models/FrmFieldFormHtml.phpclasses/models/fields/FrmFieldType.phpjs/formidable.js
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/controllers/FrmFieldsController.php
🧰 Additional context used
🧬 Code graph analysis (2)
classes/models/FrmFieldFormHtml.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5054)should_include_alert_role_on_field_errors(4013-4015)
classes/models/fields/FrmFieldType.php (1)
classes/controllers/FrmFieldsController.php (1)
input_html(531-563)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/models/FrmFieldFormHtml.php (1)
289-310: LGTM! Error shortcode handling is correct.The refactored logic properly handles three cases:
- Early exit when no error exists (lines 289-292)
- Conditional injection of
role="alert"when enabled and missing (lines 298-304)- Removal of
role="alert"when disabled but present (lines 305-306)The variable naming and logic flow are now correct and address all concerns from previous reviews.
| return 'frm_error_field_' + key.split( '-' )[ 0 ]; | ||
| } | ||
| return 'frm_error_' + input.id; | ||
| return 'frm_error_' + input.id.split( '-' )[ 0 ]; |
There was a problem hiding this comment.
@AbdiTolesa I think we want to still include part of this array.
If there is a repeater, the error IDs are something like abepx-0. So we need to only remove part of the ID and not everything right of the first dash.
There was a problem hiding this comment.
I think we only need to skip splitting the field id in the block above this line as repeating fields check goes in there and I have done that. I believe that addresses your concern.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/formidable.js (1)
1222-1259: Inconsistent multi-input handling breaks accessibility for radio/checkbox groups.The
removeFieldErrorfunction only updates the first input'saria-describedbyattribute, whileaddFieldErrorcorrectly handles all inputs. This creates an accessibility issue:Scenario: During JS validation, when a radio/checkbox group error is cleared (user fixes the validation issue), the error message is removed but
aria-describedbyattributes remain on all inputs except the first one. These orphaned attributes point to a non-existent error element, breaking screen reader announcements.Consider refactoring to use the new
updateInputElementsAriaDescribedByhelper for consistent multi-input handling:🔎 Proposed fix using the new helper function
function removeFieldError( fieldCont ) { const container = fieldCont instanceof jQuery ? fieldCont.get( 0 ) : fieldCont; if ( ! container ) { return; } const errorMessage = container.querySelector( '.frm_error' ); - const errorId = errorMessage ? errorMessage.id : ''; const input = container.querySelector( 'input, select, textarea' ); - let describedBy = input ? input.getAttribute( 'aria-describedby' ) : null; container.classList.remove( 'frm_blank_field', 'has-error' ); if ( input ) { if ( 'true' === input.getAttribute( 'aria-invalid' ) ) { input.setAttribute( 'aria-invalid', 'false' ); } else if ( [ 'radio', 'checkbox' ].includes( input.type ) ) { const group = input.closest( '[role="radiogroup"], [role="group"]' ); if ( group ) { group.setAttribute( 'aria-invalid', 'false' ); } } } if ( errorMessage ) { + updateInputElementsAriaDescribedBy( errorMessage ); errorMessage.remove(); } - - if ( input ) { - input.removeAttribute( 'aria-describedby' ); - if ( describedBy ) { - describedBy = describedBy.replace( errorId, '' ).trim(); - if ( describedBy ) { - input.setAttribute( 'aria-describedby', describedBy ); - } - } - } }
🤖 Fix all issues with AI Agents
In @js/formidable_dashboard.js:
- Around line 888-893: The code assumes querySelector found elements and adds an
event listener without guards; add null checks for userEmailInput and
subscribeButton before calling subscribeButton.addEventListener to avoid
TypeError, e.g. only attach the listener if both userEmailInput and
subscribeButton are non-null, and keep calling
_this.saveSubscribedEmail(userEmailInput.value) inside the handler (or use an
arrow function to preserve lexical this if needed); ensure the handler
references the same unique symbols userEmailInput, subscribeButton, and
saveSubscribedEmail so the listener is only registered when the elements exist.
- Around line 122-124: The beforeunload handler is passing this.cleanupObservers
directly so its this will be window when invoked; update the addEventListener
call to preserve the instance context by binding the method (e.g.,
window.addEventListener('beforeunload', this.cleanupObservers.bind(this))) or by
wrapping it in an arrow function that calls this.cleanupObservers(), and ensure
any corresponding removeEventListener uses the same bound function reference
(store the bound function on the instance, e.g., this._boundCleanupObservers) so
cleanupObservers can access this.resizeObserver correctly.
In @js/formidable.js:
- Around line 1261-1283: ESLint space-in-parens violation: remove extraneous
spaces inside parentheses in updateInputElementsAriaDescribedBy; e.g. change
calls and callbacks like document.querySelectorAll( `[aria-describedby*="${
el.id }"]` ), input.getAttribute( 'aria-describedby' ).split( ' ' ), the
.filter( value => { ... } ) callback, and input.setAttribute(
'aria-describedby', ariaDescribedBy.join( ' ' ) ) to use no inner spaces (e.g.
`[aria-describedby*="${el.id}"]`, getAttribute('aria-describedby'), .split(' '),
.filter(value => { ... }), setAttribute('aria-describedby',
ariaDescribedBy.join(' '))). Ensure all parentheses in
updateInputElementsAriaDescribedBy follow the project's space-in-parens rule.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
js/addons-page.js.mapis excluded by!**/*.map,!**/*.mapjs/form-templates.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_admin.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_blocks.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_dashboard.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_overlay.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_styles.js.mapis excluded by!**/*.map,!**/*.mapjs/onboarding-wizard.js.mapis excluded by!**/*.map,!**/*.map
📒 Files selected for processing (15)
css/admin/frm-settings-components.csscss/admin/welcome-tour.csscss/frm_testing_mode.cssjs/addons-page.jsjs/form-templates.jsjs/formidable-settings-components.jsjs/formidable.jsjs/formidable_admin.jsjs/formidable_blocks.jsjs/formidable_dashboard.jsjs/formidable_overlay.jsjs/formidable_styles.jsjs/frm_testing_mode.jsjs/onboarding-wizard.jsjs/welcome-tour.js
✅ Files skipped from review due to trivial changes (2)
- css/frm_testing_mode.css
- css/admin/frm-settings-components.css
🧰 Additional context used
🧬 Code graph analysis (1)
js/formidable_dashboard.js (3)
js/src/core/utils/animation.js (1)
frmAnimate(1-76)js/src/core/utils/error.js (1)
span(26-26)js/src/core/utils/globalModules.js (1)
bold(6-6)
🪛 Biome (2.1.2)
js/formidable_overlay.js
[error] 16-16: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 224-224: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 527-527: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
js/formidable_dashboard.js
[error] 15-15: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 83-83: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 285-285: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 588-588: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 848-848: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
🪛 GitHub Actions: Inspections
js/formidable.js
[error] 1275-1275: eslint: There must be a space before this paren (space-in-parens) at /home/runner/work/formidable-forms/formidable-forms/js/formidable.js:1275:5
🪛 GitHub Check: Run ESLint
js/formidable.js
[failure] 1275-1275:
There must be a space before this paren
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
🔇 Additional comments (5)
css/admin/welcome-tour.css (1)
1-152: Verify whether this file should be in this PR.This CSS file is unrelated to the PR's objective of adding
aria-describedbyattributes to form fields (Checkbox, Radio, Dropdown). The actual accessibility changes are inFrmFieldType.php,FrmFieldFormHtml.php,radio.js, and related field classes. Since the commit message indicates "Fix merge conflicts," this CSS file may have been accidentally included during merge resolution and should likely be removed.[scope_verification]
js/formidable_overlay.js (1)
1-788: Build output file - static analysis warnings are false positives.This is a webpack-bundled output file. The Biome warnings about function reassignment (lines 16, 224, 527) are false positives caused by Babel's standard transpilation pattern for helper functions like
_typeof. These self-reassigning helpers are intentional and safe.If source files are available for review, those would be preferable for code review rather than the compiled bundle.
js/formidable_dashboard.js (1)
860-958: Dashboard initialization pattern is well-structured overall.The
frmDashboardclass demonstrates good practices ininitCloseWelcomeBanner(lines 906-918) with proper null checks before adding event listeners. Consider applying the same defensive pattern toinitInbox.js/formidable.js (2)
1156-1198: LGTM: Multi-input support correctly implemented.The updated
addFieldErrorfunction now properly handles fields with multiple inputs (radio buttons, checkboxes). By iterating over all inputs to setaria-describedbyandaria-invalid, screen readers will correctly associate error messages with each input in a group.
1285-1294: LGTM: Proper ARIA cleanup for AJAX submissions.The
removeAllErrorsfunction now correctly removes error IDs from all inputs'aria-describedbyattributes by callingupdateInputElementsAriaDescribedBybefore removing each error element. This ensures accessibility attributes are properly cleaned up during AJAX form submissions.
| // Cleanup observers when page unloads to prevent memory leaks | ||
| window.addEventListener('beforeunload', this.cleanupObservers); | ||
| } |
There was a problem hiding this comment.
this context lost in beforeunload event handler.
"bind causes a fixed this context to be assigned" - when this.cleanupObservers is passed directly to addEventListener, the this context will be window when invoked, not the frmTabsNavigator instance. The cleanupObservers method references this.resizeObserver (line 187), which will be undefined in the wrong context.
🔎 Proposed fix using bind
-window.addEventListener('beforeunload', this.cleanupObservers);
+window.addEventListener('beforeunload', this.cleanupObservers.bind(this));Or use an arrow function wrapper:
-window.addEventListener('beforeunload', this.cleanupObservers);
+window.addEventListener('beforeunload', () => this.cleanupObservers());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Cleanup observers when page unloads to prevent memory leaks | |
| window.addEventListener('beforeunload', this.cleanupObservers); | |
| } | |
| // Cleanup observers when page unloads to prevent memory leaks | |
| window.addEventListener('beforeunload', () => this.cleanupObservers()); | |
| } |
🤖 Prompt for AI Agents
In @js/formidable_dashboard.js around lines 122 - 124, The beforeunload handler
is passing this.cleanupObservers directly so its this will be window when
invoked; update the addEventListener call to preserve the instance context by
binding the method (e.g., window.addEventListener('beforeunload',
this.cleanupObservers.bind(this))) or by wrapping it in an arrow function that
calls this.cleanupObservers(), and ensure any corresponding removeEventListener
uses the same bound function reference (store the bound function on the
instance, e.g., this._boundCleanupObservers) so cleanupObservers can access
this.resizeObserver correctly.
| var userEmailInput = document.querySelector('#frm_leave_email'); | ||
| var subscribeButton = document.querySelector('#frm-add-my-email-address'); | ||
| subscribeButton.addEventListener('click', function () { | ||
| _this.saveSubscribedEmail(userEmailInput.value).then(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing null checks before adding event listeners.
this refers to the element when a method is passed as a callback, but more critically, subscribeButton and userEmailInput are not checked for null before use. If these elements don't exist in the DOM, addEventListener will throw a TypeError.
🔎 Proposed fix with null guards
var userEmailInput = document.querySelector('#frm_leave_email');
var subscribeButton = document.querySelector('#frm-add-my-email-address');
+if (!subscribeButton || !userEmailInput) {
+ return;
+}
subscribeButton.addEventListener('click', function () {
_this.saveSubscribedEmail(userEmailInput.value).then();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var userEmailInput = document.querySelector('#frm_leave_email'); | |
| var subscribeButton = document.querySelector('#frm-add-my-email-address'); | |
| subscribeButton.addEventListener('click', function () { | |
| _this.saveSubscribedEmail(userEmailInput.value).then(); | |
| }); | |
| } | |
| var userEmailInput = document.querySelector('#frm_leave_email'); | |
| var subscribeButton = document.querySelector('#frm-add-my-email-address'); | |
| if (!subscribeButton || !userEmailInput) { | |
| return; | |
| } | |
| subscribeButton.addEventListener('click', function () { | |
| _this.saveSubscribedEmail(userEmailInput.value).then(); | |
| }); |
🤖 Prompt for AI Agents
In @js/formidable_dashboard.js around lines 888 - 893, The code assumes
querySelector found elements and adds an event listener without guards; add null
checks for userEmailInput and subscribeButton before calling
subscribeButton.addEventListener to avoid TypeError, e.g. only attach the
listener if both userEmailInput and subscribeButton are non-null, and keep
calling _this.saveSubscribedEmail(userEmailInput.value) inside the handler (or
use an arrow function to preserve lexical this if needed); ensure the handler
references the same unique symbols userEmailInput, subscribeButton, and
saveSubscribedEmail so the listener is only registered when the elements exist.
| /** | ||
| * Updates the aria-describedby attribute of input elements to remove the error element ID. | ||
| * | ||
| * @since x.x | ||
| * | ||
| * @param {HTMLElement} el The error element. | ||
| * @return {void} | ||
| */ | ||
| function updateInputElementsAriaDescribedBy( el ) { | ||
| document.querySelectorAll( `[aria-describedby*="${ el.id }"]` ).forEach( input => { | ||
| let ariaDescribedBy = input.getAttribute( 'aria-describedby' ).split( ' ' ); | ||
| ariaDescribedBy = ariaDescribedBy.filter( value => { | ||
| const trimmedValue = value.trim(); | ||
| return trimmedValue && trimmedValue !== el.id; | ||
| }); | ||
|
|
||
| if ( ariaDescribedBy.length ) { | ||
| input.setAttribute( 'aria-describedby', ariaDescribedBy.join( ' ' ) ); | ||
| return; | ||
| } | ||
| input.removeAttribute( 'aria-describedby' ); | ||
| } ); | ||
| } |
There was a problem hiding this comment.
Fix ESLint spacing error blocking CI.
The updateInputElementsAriaDescribedBy helper function correctly handles removing error IDs from all inputs' aria-describedby attributes. However, there's a spacing violation on line 1275 preventing the build from passing.
🔎 Fix for ESLint space-in-parens violation
function updateInputElementsAriaDescribedBy( el ) {
document.querySelectorAll( `[aria-describedby*="${ el.id }"]` ).forEach( input => {
let ariaDescribedBy = input.getAttribute( 'aria-describedby' ).split( ' ' );
ariaDescribedBy = ariaDescribedBy.filter( value => {
const trimmedValue = value.trim();
return trimmedValue && trimmedValue !== el.id;
- });
+ } );
if ( ariaDescribedBy.length ) {
input.setAttribute( 'aria-describedby', ariaDescribedBy.join( ' ' ) );
return;
}
input.removeAttribute( 'aria-describedby' );
} );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Updates the aria-describedby attribute of input elements to remove the error element ID. | |
| * | |
| * @since x.x | |
| * | |
| * @param {HTMLElement} el The error element. | |
| * @return {void} | |
| */ | |
| function updateInputElementsAriaDescribedBy( el ) { | |
| document.querySelectorAll( `[aria-describedby*="${ el.id }"]` ).forEach( input => { | |
| let ariaDescribedBy = input.getAttribute( 'aria-describedby' ).split( ' ' ); | |
| ariaDescribedBy = ariaDescribedBy.filter( value => { | |
| const trimmedValue = value.trim(); | |
| return trimmedValue && trimmedValue !== el.id; | |
| }); | |
| if ( ariaDescribedBy.length ) { | |
| input.setAttribute( 'aria-describedby', ariaDescribedBy.join( ' ' ) ); | |
| return; | |
| } | |
| input.removeAttribute( 'aria-describedby' ); | |
| } ); | |
| } | |
| /** | |
| * Updates the aria-describedby attribute of input elements to remove the error element ID. | |
| * | |
| * @since x.x | |
| * | |
| * @param {HTMLElement} el The error element. | |
| * @return {void} | |
| */ | |
| function updateInputElementsAriaDescribedBy( el ) { | |
| document.querySelectorAll( `[aria-describedby*="${ el.id }"]` ).forEach( input => { | |
| let ariaDescribedBy = input.getAttribute( 'aria-describedby' ).split( ' ' ); | |
| ariaDescribedBy = ariaDescribedBy.filter( value => { | |
| const trimmedValue = value.trim(); | |
| return trimmedValue && trimmedValue !== el.id; | |
| } ); | |
| if ( ariaDescribedBy.length ) { | |
| input.setAttribute( 'aria-describedby', ariaDescribedBy.join( ' ' ) ); | |
| return; | |
| } | |
| input.removeAttribute( 'aria-describedby' ); | |
| } ); | |
| } |
🧰 Tools
🪛 GitHub Actions: Inspections
[error] 1275-1275: eslint: There must be a space before this paren (space-in-parens) at /home/runner/work/formidable-forms/formidable-forms/js/formidable.js:1275:5
🪛 GitHub Check: Run ESLint
[failure] 1275-1275:
There must be a space before this paren
🤖 Prompt for AI Agents
In @js/formidable.js around lines 1261 - 1283, ESLint space-in-parens violation:
remove extraneous spaces inside parentheses in
updateInputElementsAriaDescribedBy; e.g. change calls and callbacks like
document.querySelectorAll( `[aria-describedby*="${ el.id }"]` ),
input.getAttribute( 'aria-describedby' ).split( ' ' ), the .filter( value => {
... } ) callback, and input.setAttribute( 'aria-describedby',
ariaDescribedBy.join( ' ' ) ) to use no inner spaces (e.g.
`[aria-describedby*="${el.id}"]`, getAttribute('aria-describedby'), .split(' '),
.filter(value => { ... }), setAttribute('aria-describedby',
ariaDescribedBy.join(' '))). Ensure all parentheses in
updateInputElementsAriaDescribedBy follow the project's space-in-parens rule.
4 participants
Fix https://github.com/Strategy11/formidable-pro/issues/6078
aria-describedbyattributes when field errors happen.aria-describedbyvalues are removed from elements when errors are cleared, during Ajax submissionRequiredin the fields options.aria-describedbyattribute is added to the elements to link them to the error message element programatically.